feat(spur-client): add client-side controller failover with endpoint rotation by shiv-tyagi · Pull Request #384 · ROCm/spur

shiv-tyagi · 2026-07-05T04:01:55Z

What

Closes #338.

spurd and the CLI could only be pointed at a single controller address, so an HA quorum offered no client-side resilience: if the one configured controller was down, connections failed even when other peers were healthy. This adds client-side failover across a list of controller endpoints.

Approach

New spur-client crate provides a service-agnostic dialer:

connect_channel(endpoints) takes a comma-separated list, tries each in order, and rotates to the next on connection failure, returning a tonic::transport::Channel. If every endpoint fails, the last error is returned.
Callers wrap the channel in whatever generated client they need (SlurmControllerClient::new(channel)), so the crate depends on neither spur-proto nor spur-core.

A single reachable controller is sufficient: server-side Raft leader forwarding already routes writes to the current leader, so the client only needs to reach any live peer.

Config plumbing:

ControllerConfig::endpoints() expands hosts + the listen_addr port into an endpoint list.
The CLI exports it as a comma-joined SPUR_CONTROLLER_ADDR; the same env var and --controller flag accept a comma-separated list.

Call sites in spur-cli, spurd, and spur-ffi switch from SlurmControllerClient::connect(addr) to building a client from connect_channel(&addr).

Design notes

Endpoints keep the http:// scheme because tonic requires a full URI for the h2c transport; entries without a scheme are normalized to http://.
The crate is intentionally minimal (only tonic + tracing) to keep layering clean and avoid pulling proto/config into the transport helper.

Testing

spur-client unit tests: endpoint parsing (scheme normalization, whitespace, empty entries) and rotation (dead-first reaches a live server, all-down returns an error).
spur-core unit tests for ControllerConfig::endpoints().
Native-host e2e (test_controller_failover.py) run against the 4-node bare-metal cluster, all passing: rotation past a dead first endpoint, live-first list unaffected by a trailing dead endpoint, all-down list fails cleanly with a connection error (no hang), and a job submitted through a failover list completes.
cargo clippy --workspace --exclude spur-ffi --all-targets clean; cargo fmt applied.
Manual 3-peer Raft failover on the bare-metal testbed: killing the first controller in the list, the CLI rotates to a survivor and commands succeed after re-election.

Full multi-controller Raft failover in the automated native-host harness is deferred; the harness needs Raft support first.

…rotation Add a service-agnostic spur-client crate that dials a tonic Channel from a comma-separated endpoint list, rotating to the next endpoint when one is unreachable. spurctld, spurd, and the CLI now build clients from this channel instead of connecting to a single hardcoded address, so a single reachable controller in an HA quorum is enough (server-side leader forwarding handles the rest). - spur-client: parse_endpoints + connect_channel, with unit tests for parsing and rotation (dead-first, all-down). - ControllerConfig::endpoints(): expand hosts + listen_addr port into an endpoint list; CLI exports it as a comma-joined SPUR_CONTROLLER_ADDR. - Refactor call sites in spur-cli, spurd, and spur-ffi to SlurmControllerClient::new(connect_channel(&addr).await?). - Docs + examples: document the comma-separated HA controller list. - Native-host e2e: test_controller_failover covers rotation past a dead endpoint, live-first lists, clean all-down failure, and submit-through-list.

codecov-commenter · 2026-07-05T04:04:52Z

Codecov Report

❌ Patch coverage is 64.55026% with 67 lines in your changes missing coverage. Please review.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #384      +/-   ##
==========================================
+ Coverage   68.22%   68.29%   +0.07%     
==========================================
  Files         134      135       +1     
  Lines       36098    36235     +137     
==========================================
+ Hits        24625    24744     +119     
- Misses      11473    11491      +18

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot

Pull request overview

This PR introduces client-side controller failover for HA deployments by adding a new spur-client crate that dials a comma-separated list of controller endpoints and updates spurd, spur-cli, and spur-ffi call sites to use it. It also adds config/documentation plumbing and E2E coverage to validate endpoint rotation behavior.

Changes:

Add crates/spur-client with endpoint parsing + connection rotation and unit tests.
Switch controller connections in spurd, spur-cli, and spur-ffi from SlurmControllerClient::connect(addr) to SlurmControllerClient::new(connect_channel(...)).
Export multi-endpoint controller addresses via config -> SPUR_CONTROLLER_ADDR, document usage, and add native-host E2E failover tests.

Reviewed changes

Copilot reviewed 30 out of 31 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
tests/native_host/e2e/test_controller_failover.py	Adds E2E coverage for comma-separated controller endpoint rotation via CLI.
tests/native_host/e2e/cluster.py	Extends CLI helpers to override `SPUR_CONTROLLER_ADDR` per invocation.
examples/spur.conf	Documents `controller.hosts` HA configuration for client-side failover.
docs/deployment/native-host.rst	Documents comma-separated `--controller` / `SPUR_CONTROLLER_ADDR` usage for HA.
crates/spurd/src/reporter.rs	Uses `spur_client::connect_channel` for registration/deregistration/heartbeat connections.
crates/spurd/src/agent_server.rs	Uses `spur_client::connect_channel` when reporting job completion to controllers.
crates/spurd/Cargo.toml	Adds `spur-client` dependency to `spurd`.
crates/spur-ffi/src/lib.rs	Routes FFI controller calls through `spur_client::connect_channel`.
crates/spur-ffi/Cargo.toml	Adds `spur-client` dependency to `spur-ffi`.
crates/spur-core/src/config.rs	Adds `ControllerConfig::endpoints()` and unit tests for endpoint expansion.
crates/spur-client/src/lib.rs	New failover dialer: parse/normalize endpoints, rotate on failure, unit tests.
crates/spur-client/Cargo.toml	New crate manifest and dev-deps for transport tests.
crates/spur-cli/src/token.rs	Switches token subcommands to build client from `connect_channel`.
crates/spur-cli/src/sstat.rs	Switches `sstat` controller connection to `connect_channel`.
crates/spur-cli/src/srun.rs	Switches `srun` controller connections to `connect_channel`.
crates/spur-cli/src/squeue.rs	Switches `squeue` controller connection to `connect_channel`.
crates/spur-cli/src/sprio.rs	Switches `sprio` controller connection to `connect_channel`.
crates/spur-cli/src/smd.rs	Switches `smd` controller connection to `connect_channel`.
crates/spur-cli/src/sinfo.rs	Switches `sinfo` controller connection to `connect_channel`.
crates/spur-cli/src/sdiag.rs	Switches `sdiag` controller connection to `connect_channel`.
crates/spur-cli/src/scontrol.rs	Switches `scontrol` controller connections to `connect_channel`.
crates/spur-cli/src/scancel.rs	Switches `scancel` controller connection to `connect_channel`.
crates/spur-cli/src/sbatch.rs	Switches `sbatch` controller connection to `connect_channel`.
crates/spur-cli/src/sattach.rs	Switches `sattach` controller connection to `connect_channel`.
crates/spur-cli/src/salloc.rs	Switches `salloc` controller connection to `connect_channel`.
crates/spur-cli/src/node.rs	Switches `node` subcommands controller connections to `connect_channel`.
crates/spur-cli/src/main.rs	Loads config-derived multi-endpoint controller list into `SPUR_CONTROLLER_ADDR`.
crates/spur-cli/src/exec.rs	Switches `exec` controller connection to `connect_channel`.
crates/spur-cli/Cargo.toml	Adds `spur-client` dependency to `spur-cli`.
Cargo.toml	Adds `crates/spur-client` to the workspace members.
Cargo.lock	Records new crate/dependency resolution from adding `spur-client` + dev-deps.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+async fn try_connect(endpoint: &str) -> Result<Channel, tonic::transport::Error> {
+    Endpoint::from_shared(endpoint.to_string())?.connect().await
+}


+    pub fn endpoints(&self) -> Vec<String> {
+        let port = self.listen_addr.rsplit(':').next().unwrap_or("6817");
+        self.hosts
+            .iter()
+            .map(|host| format!("http://{host}:{port}"))
+            .collect()
+    }


+            Err(e) => warn!(
+                %endpoint,
+                error = %e,
+                "controller endpoint unreachable, trying next"
+            ),


yansun1996

Please see the inline comments below.

yansun1996 · 2026-07-06T20:36:53Z

+}
+
+async fn try_connect(endpoint: &str) -> Result<Channel, tonic::transport::Error> {
+    Endpoint::from_shared(endpoint.to_string())?.connect().await


Endpoint::from_shared(...).connect() has no connect timeout. tonic/hyper set none by default, so an endpoint unreachable via a network partition (dropped packets, not an active refusal) can hang for the OS TCP timeout (~2 min on Linux) before rotating to the next endpoint — defeating the point of fast failover. crates/spurctld/src/raft.rs already calls .connect_timeout(Duration::from_secs(2)) for exactly this reason; worth mirroring that here.

yansun1996 · 2026-07-06T20:36:53Z

+/// Once connected to any running controller, server-side leader forwarding
+/// routes writes to the current Raft leader, so a single reachable node is
+/// sufficient regardless of which one is the leader.
+pub async fn connect_channel(endpoints: &str) -> Result<Channel, tonic::transport::Error> {


Nit/follow-up: no memoization of the last-known-good endpoint — every call starts at index 0, so long-lived periodic callers (e.g. the heartbeat loop) will keep re-dialing a dead first endpoint every cycle instead of remembering the one that worked last time. Not a correctness issue given the intentionally stateless design, but worth tracking as a follow-up for steady-state HA efficiency.

yansun1996 · 2026-07-06T20:36:53Z

+        let port = self.listen_addr.rsplit(':').next().unwrap_or("6817");
+        self.hosts
+            .iter()
+            .map(|host| format!("http://{host}:{port}"))


Nit: format!("http://{host}:{port}") produces a malformed URI if host is a bare IPv6 literal (e.g. ::1), since it isn't bracketed (needs [::1]:6817). Low priority if IPv6 controller hosts aren't a supported deployment target, but worth a doc note or bracket-check otherwise.

yansun1996 · 2026-07-06T20:36:53Z

+    }
+
+    #[tokio::test]
+    async fn connect_rotates_to_first_reachable() {


Test coverage: this only exercises rotation past a single dead endpoint. Worth adding a case with two consecutive dead endpoints before the live one (dead1, dead2, up), closer to the 3-node HA topology this feature targets. Can reuse free_addr()/spawn_server() as-is — no new scaffolding needed.

yansun1996 · 2026-07-06T20:36:53Z

    use super::*;

+    #[test]
+    fn test_controller_endpoints_single_host() {


Test coverage: no case for hosts: vec![], which is the runtime state main.rs's if !endpoints.is_empty() branch guards against. A test_controller_endpoints_empty_hosts asserting endpoints() returns [] for empty hosts would close that gap.

shiv-tyagi requested review from powderluv and sgopinath1 as code owners July 5, 2026 04:01

Copilot AI review requested due to automatic review settings July 5, 2026 04:01

shiv-tyagi requested review from sajmera-pensando and yansun1996 as code owners July 5, 2026 04:01

Copilot started reviewing on behalf of shiv-tyagi July 5, 2026 04:02 View session

Copilot AI reviewed Jul 5, 2026

View reviewed changes

yansun1996 reviewed Jul 6, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(spur-client): add client-side controller failover with endpoint rotation#384

feat(spur-client): add client-side controller failover with endpoint rotation#384
shiv-tyagi wants to merge 1 commit into
ROCm:mainfrom
shiv-tyagi:feat/multi-controller-failover

shiv-tyagi commented Jul 5, 2026

Uh oh!

codecov-commenter commented Jul 5, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

yansun1996 left a comment •

edited

Loading

Uh oh!

yansun1996 Jul 6, 2026

Uh oh!

yansun1996 Jul 6, 2026

Uh oh!

yansun1996 Jul 6, 2026

Uh oh!

yansun1996 Jul 6, 2026

Uh oh!

yansun1996 Jul 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

shiv-tyagi commented Jul 5, 2026

What

Approach

Design notes

Testing

Uh oh!

codecov-commenter commented Jul 5, 2026

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

yansun1996 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yansun1996 Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

yansun1996 Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

yansun1996 Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

yansun1996 Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

yansun1996 Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

yansun1996 left a comment •

edited

Loading